Skip to content

Return ValueError instead of panicking on duplicate mergeable_ranks#579

Open
devYRPauli wants to merge 1 commit into
openai:mainfrom
devYRPauli:fix/duplicate-ranks-valueerror
Open

Return ValueError instead of panicking on duplicate mergeable_ranks#579
devYRPauli wants to merge 1 commit into
openai:mainfrom
devYRPauli:fix/duplicate-ranks-valueerror

Conversation

@devYRPauli

Copy link
Copy Markdown

Problem

Constructing an Encoding with duplicate ranks in mergeable_ranks crashes with an uncatchable pyo3_runtime.PanicException instead of raising a normal ValueError:

import tiktoken
tiktoken.Encoding(name="x", pat_str=r".", mergeable_ranks={b"a": 0, b"b": 0, b"c": 1}, special_tokens={})
# pyo3_runtime.PanicException: Encoder and decoder must be of equal length. ...

Because it is a panic rather than an exception, callers cannot handle it with except ValueError (or any normal except), and it can tear down the interpreter.

Root cause

In CoreBPE::new_internal (src/lib.rs), the length mismatch between the encoder and the derived decoder - which happens when two tokens share a rank - is checked with a raw assert!. new_internal already returns Result<Self, ...> and surfaces regex errors cleanly via Regex::new(pattern)?, and py.rs maps that Err to PyValueError. The assert! panics before it can return through that path.

Fix

Return an Err on the length mismatch so it flows through the existing Result path (and therefore the PyValueError mapping in py.rs) instead of panicking. The message is unchanged.

Test

Adds tests/test_duplicate_ranks.py, which asserts that duplicate mergeable_ranks raise ValueError. Before the change it fails with PanicException; after, it passes.

Refs #87 (same crash, reported in 2023).

Constructing an Encoding with duplicate ranks in mergeable_ranks made
CoreBPE::new_internal hit a raw assert!, which surfaces to Python as an
uncatchable pyo3_runtime.PanicException instead of a clean ValueError.

new_internal already returns Result and reports regex errors cleanly via
Regex::new(pattern)?; py.rs maps that Err to PyValueError. Return an Err
on the length mismatch so it flows through the same path.

Refs openai#87.

Signed-off-by: Yash Raj Pandey <yashpn62@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant